Skip to content

fix: re-enable RPC tests and modernize harness (#5540)#5632

Merged
thomhurst merged 1 commit intomainfrom
fix/rpc-tests-issue-5540
Apr 19, 2026
Merged

fix: re-enable RPC tests and modernize harness (#5540)#5632
thomhurst merged 1 commit intomainfrom
fix/rpc-tests-issue-5540

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Root cause fix: RunTestsRequest was serializing the node filter as testCases, but Microsoft.Testing.Platform reads it from tests — so the server silently ignored the filter and ran the full catalogue on every RunTests call. Renamed the JSON property to match the protocol wire format.
  • Pipeline TFM: RunRpcTestsModule targeted net8.0 while TUnit.RpcTests.csproj only targets net10.0, leaving the suite skipped. Aligned the testable framework list to net10.0.
  • Modernization: extracted TestHostSession (IAsyncDisposable) and TestProjectBuilds (per-framework shared build cache), parameterized the three scenarios across net8.0/net10.0 via MethodDataSource, removed dead abstractions (IProcessHandle/ProcessHandle, custom LogLevel enum, ConsoleRpcListener, RunRequest), switched to Microsoft.Extensions.Logging.LogLevel with a string-enum converter, and tightened TestNode deserialization via JsonExtensionData.

Closes #5540.

Test plan

  • Discovery_ReturnsFullTestCatalogue passes on net8.0 and net10.0
  • RunTests_WithUidFilter_ExecutesOnlySelectedTests passes on net8.0 and net10.0 — confirms server honors the filter
  • RunTests_WithSkippedTest_ReportsSkippedState passes on net8.0 and net10.0
  • Full local run: 6/6 green in ~2m22s
  • dotnet build TUnit.RpcTests clean (0 warnings, 0 errors)

Restore the previously-disabled RPC test suite and fix the underlying
failures. The MTP server was silently ignoring the client's test filter
because the RunTestsRequest wire field was serialized as `testCases`, but
Microsoft.Testing.Platform reads it from `tests`. This caused every
`RunTests` call to execute the full catalogue instead of the selected
nodes. Renamed the JSON property to match the protocol.

Pipeline was also targeting `net8.0` while the csproj only targets
`net10.0`, so the suite never ran. Aligned the testable framework list.

While re-enabling, modernized the harness: extracted TestHostSession
(IAsyncDisposable) and TestProjectBuilds (shared per-framework build
cache), parameterized tests across net8.0/net10.0 via MethodDataSource,
dropped dead abstractions (IProcessHandle/ProcessHandle, custom LogLevel,
ConsoleRpcListener, RunRequest), and switched to
Microsoft.Extensions.Logging.LogLevel with a string-enum converter.
@codacy-production
Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
ErrorProne 1 medium

View in Codacy

🟢 Metrics 9 complexity

Metric Results
Complexity 9

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This PR is a solid fix — the root cause ("testCases""tests" wire-format mismatch) is clear and correct, and the refactoring around it is well-motivated.

Core Fix ✅

RunTestsRequest.cs — The JSON property rename from testCases to tests is the crux of the fix. The protocol was silently ignoring the filter because the server read a field the client wasn't sending under that name. Correct.

RunRpcTestsModule.cs — Re-enabling net10.0 to match the project target is correct. No concerns here.


Architecture: TestHostSession

Extracting session lifecycle into an IAsyncDisposable is the right abstraction. The private constructor + static factory (StartAsync) pattern is idiomatic for types that require async initialization. Good.

One issue in DisposeAsync: Client.Dispose() almost certainly disposes _tcpClient internally (since TestingPlatformClient still holds a reference to it), and then _tcpClient.Dispose() is called a second time. TcpClient tolerates double-dispose, but it is a code smell. Consider having TestingPlatformClient.Dispose() only dispose the JsonRpc/stream, and let TestHostSession own _tcpClient disposal exclusively. Similarly, _rpc.Dispose() is called after Client.Dispose() — if TestingPlatformClient disposes the JsonRpc it receives, that is another double-dispose.


TestProjectBuilds.EnsureBuiltAsync — Stale-failure risk

public static Task EnsureBuiltAsync(string framework, CancellationToken cancellationToken)
    => _builds.GetOrAdd(framework, tfm => BuildAsync(tfm, cancellationToken));

If the first caller is cancelled (or the build fails transiently), the faulted/cancelled Task is permanently cached in _builds. Every subsequent caller for that TFM will immediately receive the same bad task instead of retrying. In a flaky CI environment this could permanently poison the test run. A safer pattern:

public static Task EnsureBuiltAsync(string framework, CancellationToken cancellationToken)
{
    if (_builds.TryGetValue(framework, out var existing) && !existing.IsFaulted && !existing.IsCanceled)
        return existing;
    var fresh = BuildAsync(framework, cancellationToken);
    _builds[framework] = fresh;
    return fresh;
}

There is also a subtle race: GetOrAdd is not atomic — two threads may both call BuildAsync for the same TFM simultaneously. For a test helper this is probably harmless (both builds succeed), but worth noting.


Frameworks() Return Type

public static IEnumerable<Func<string>> Frameworks()
{
    yield return () => "net8.0";
    yield return () => "net10.0";
}

The test methods accept string framework, not Func<string>. If TUnit's MethodDataSource transparently invokes the Func<T> wrapper to materialize the argument, this works — but it is not obvious from reading the code why wrapping in Func<T> is needed here. If TUnit supports IEnumerable<string> directly from a MethodDataSource, prefer that for clarity. If the Func<string> wrapper is load-bearing (e.g., deferred evaluation), a comment explaining why would help future readers.


ExitAsync Silent Swallow

catch
{
    // Best effort — connection may already be gone
}

Swallowing all exceptions unconditionally can hide bugs during development. Since this is called in DisposeAsync, consider at least logging to Console.Error or Trace so it is visible in test output without being fatal.


TestNode.JsonExtensionData

Using [JsonExtensionData] to round-trip unknown fields back to the server when constructing RunTestsRequest is exactly right. This avoids the common trap of stripping server-generated fields (traits, location info, etc.) that the server expects to see back on runTests.


Dead Code Removal ✅

Removing IProcessHandle/ProcessHandle (which had a NotImplementedException in StopAsync and no-ops everywhere else), ConsoleRpcListener, the duplicate LogLevel enum, and RunRequest are all correct.


Minor: Magic number

await Assert.That(discovered).Count().IsGreaterThanOrEqualTo(4000);

This will fail if the test project's test count drops below 4000. Consider a named constant or a comment explaining where this threshold comes from.


Summary: The fix is correct and the refactoring is a clear improvement. The two issues worth addressing before merge are the stale-task caching in TestProjectBuilds (can permanently break a CI run after a transient failure) and the potential double-dispose in TestHostSession. Everything else is minor.

@thomhurst thomhurst merged commit 1237ee3 into main Apr 19, 2026
14 of 15 checks passed
@thomhurst thomhurst deleted the fix/rpc-tests-issue-5540 branch April 19, 2026 19:35
This was referenced Apr 20, 2026
This was referenced Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix and re-enable RPC tests in CI pipeline

1 participant